Enhancement lastUpdated cell styling #135

wrigh393 · 2020-12-03T01:31:01Z

No description provided.

…to control cell color based on date of last update

…variable to make more specific and clear.

…f items last update.

campb303 · 2020-12-03T01:52:59Z

Closes #90

campb303

Logically this works well. Formatting needs some work.

src/components/ItemTable/ItemTable.js

campb303 · 2020-12-03T01:54:42Z

src/components/ItemTable/ItemTable.js

+        return (
+            <TableCell
+                classes={classes}
+                {...reactTableCellProps} //expands table cell props to allow access to style props


Move comment above line of code it applies to. Use a space between // and the start of the comment. Use proper sentence case with comments.

Suggested change

{...reactTableCellProps} //expands table cell props to allow access to style props

// Expands table cell props to allow access to style props.

{...reactTableCellProps}

campb303 · 2020-12-03T01:56:04Z

src/components/ItemTable/ItemTable.js

+            <TableCell
+                classes={classes}
+                {...reactTableCellProps} //expands table cell props to allow access to style props
+                style={{ ...reactTableCellProps.style, backgroundColor: backgroundColor }} //expands style prop of table cell to allow addtion of custom styling without losing default styling


Move comment above line of code it applies to. Use a space between // and the start of the comment. Use proper sentence case with comments.

Suggested change

style={{ ...reactTableCellProps.style, backgroundColor: backgroundColor }} //expands style prop of table cell to allow addtion of custom styling without losing default styling

// Expands style prop of table cell to allow addition of custom styling without losing default styling.

style={{ ...reactTableCellProps.style, backgroundColor: backgroundColor }}

campb303 · 2020-12-03T01:57:10Z

src/components/ItemTable/ItemTable.js

@@ -34,7 +65,8 @@ export default function ItemTable({ data }) {
            borderLeftWidth: "1px",
            borderLeftStyle: "solid",
            borderColor: theme.palette.type === "light" ? theme.palette.grey[300] : theme.palette.grey[500]
-        }
+        },
+


Remove unnecessary blank line.

campb303 · 2020-12-03T01:58:18Z

src/components/ItemTable/ItemTable.js

@@ -48,7 +80,7 @@ export default function ItemTable({ data }) {
            { Header: 'Subject', accessor: 'subject' },
            { Header: 'Status', accessor: 'status', },
            { Header: 'Priority', accessor: 'priority' },
-            { Header: 'Last Updated', accessor: 'lastUpdated', Cell: ({ value }) => <RelativeTime value={value} /> },
+            { Header: 'Last Updated', accessor: 'lastUpdated', Cell: ({ value }) => <LastUpdatedCell time={value} /> },


Remove Cell overrides because we're controlling them in the render statements below. The definitions here will never be used.

campb303 · 2020-12-03T01:59:44Z

src/components/ItemTable/ItemTable.js

+            <Table {...getTableProps}
+                aria-label="Table of Queue Items"


When splitting component props across multiple lines, do not include props on the same line as the opening tag.

Suggested change

<Table {...getTableProps}

aria-label="Table of Queue Items"

<Table

{...getTableProps}

aria-label="Table of Queue Items"

campb303 · 2020-12-03T02:01:19Z

src/components/ItemTable/ItemTable.js

@@ -135,9 +170,24 @@ export default function ItemTable({ data }) {
                                selected={isSelected}
                                {...row.getRowProps()} >


Like with starting tags and multi-line props, place tag endings on their own line.

Suggested change

{...row.getRowProps()} >

{...row.getRowProps()}

>

campb303 · 2020-12-03T02:02:09Z

src/components/ItemTable/ItemTable.js

-                                    </TableCell>
+
+                                    cell.render(
+                                        //conditonally renders custom cell component based on cell.column.id prop value


Use a space between // and the start of the comment. Use proper sentence case with comments.

Suggested change

//conditonally renders custom cell component based on cell.column.id prop value

// Conditonally renders custom cell component based on cell.column.id prop value.

campb303 · 2020-12-03T02:04:22Z

src/components/ItemTable/ItemTable.js

+                                        cell.column.id === "lastUpdated"
+                                            ? <LastUpdatedCell
+                                                time={cell.value}
+                                                reactTableCellProps={cell.getCellProps()}
+                                                classes={{ root: classes.columnBorders }}
+                                            />
+                                            :
+                                            <TableCell
+                                                {...cell.getCellProps()}
+                                                classes={{ root: classes.columnBorders }}
+                                            >


When using multi line values for ternary operators, wrap the value for true and false in parenthesis.

Suggested change

cell.column.id === "lastUpdated"

? <LastUpdatedCell

time={cell.value}

reactTableCellProps={cell.getCellProps()}

classes={{ root: classes.columnBorders }}

/>

:

<TableCell

{...cell.getCellProps()}

classes={{ root: classes.columnBorders }}

>

cell.column.id === "lastUpdated"

? (

<LastUpdatedCell

time={cell.value}

reactTableCellProps={cell.getCellProps()}

classes={{ root: classes.columnBorders }}

/>

)

: (

<TableCell

{...cell.getCellProps()}

classes={{ root: classes.columnBorders }}

>

)

campb303 · 2020-12-03T02:04:39Z

src/components/ItemTable/ItemTable.js

+                                                {cell.value}
+                                            </TableCell>
+                                    )
+


Remove extra blank line.

…render,

campb303

Logic looks good. Needs refactoring.

campb303 · 2020-12-15T18:41:50Z

src/components/ItemTable/ItemTable.js

+    const LastUpdatedCell = ({ time, reactTableCellProps, classes }) => {
+        const lastUpdated = new Date(time).getTime();
+        const now = new Date().getTime();
+        const timeDelta = now - lastUpdated;
+        const day = 24 * 60 * 60 * 1000;
+        const week = day * 7;
+        const month = week * 4;
+
+        let backgroundColor = "white";
+        if (timeDelta > day && timeDelta <= week) {
+            backgroundColor = red[100]
+        }
+        else if (timeDelta > week && timeDelta <= month) {
+            backgroundColor = red[300]
+        }
+        else if (timeDelta > month) {
+            backgroundColor = red[500]
+        }
+
+        return (
+            <TableCell
+                classes={classes}
+                // Expands table cell props to allow access to style props.
+                {...reactTableCellProps}
+                // Expands style prop of table cell to allow addtion of custom styling without losing default styling.
+                style={{ ...reactTableCellProps.style, backgroundColor: backgroundColor }}
+            >
+                <RelativeTime value={time} />
+            </TableCell>
+        );
+    };


Logic is good. This should be its own component with docs. It should also extend an ItemTableCell (see comments below about ItemTableCell.)

campb303 · 2020-12-15T18:47:31Z

src/components/ItemTable/ItemTable.js

+                                                <TableCell
+                                                    {...cell.getCellProps()}
+                                                    classes={{ root: classes.columnBorders }}
+                                                >
+                                                    {cell.value}
+                                                </TableCell>


We want to avoid WET (writing everything twice) code and keep it DRY (don't repeat yourself). We can do that here by making a component for a TableCell and giving it the CSS override equivalent to classes.columnBorders. It could be called something like ItemTableCell. Then we'd have one and only one place where we need to make styling changes.

Something like this could also be extended by the LastUpdatedCell so we're not duplicating logic.

… ItemTable

(cherry picked from commit 47e0715)

(cherry picked from commit 5dd1da9)

campb303 · 2021-01-21T18:29:11Z

A significant amount of work needed to be put in to resolve some logical, visual, and organizational issues.

Logical

With more than 2 options, ternary statements must be nested and this is difficult to read. switch statements and if/elif/else trees are better (in that order) for readability with multiple options. ItemTable's TableBody generation was updated to use a switch statement instead of nested ternary statements to improve readability.

The LastUpdatedCell had styling logic from the ItemTableCell duplicated unnecessarily which would lead to breaking changes in the future. The LastUpdatedCell is not effectively inheriting from the LastUpdatedCell to avoid this issue.

The logic in the LastUpdatedCell that determines the background color was moved to its own function to better isolate code fragments. This also made it easier to inset the background color into the styling info from React Table.

Visual

Because of inheritance and overriding issues with styling, the sizing style rules were not being correctly applied to the LastUpdatedCell. This was fixed by injecting backgroundColor into the props object passed from React Table.

Because LastUpdatedCell was duplicating ItemTableCell's border information, some borders were being doubled up. This was fixed with proper inheritance.

Organizational

Documentation for ItemTableCell and LastUpdatedCell were not created. This was done now but docs should always be made at the same time as the component.

Code formatting was inconsistent. This was fixed by using the jslint formatter from within VSCode.

Tyler Jordan Wright added 4 commits November 23, 2020 13:31

Added background state varible and created lastUpdatedCell component …

6e6492b

…to control cell color based on date of last update

Removed uncesscary function and variables. Changed name of the state …

a6d8132

…variable to make more specific and clear.

Implemented logic for changing cell color based on last updated time.

ab75000

implemented logic that sets color of lastUpdated cell based on time o…

6b67902

…f items last update.

campb303 requested changes Dec 3, 2020

View reviewed changes

Forammting fixes, removed uneccessary overrides, fixed date received …

c418a5a

…render,

wrigh393 added frontend enhancement Request for a change to existing functionality high-priority Needs immediate extra focus labels Dec 3, 2020

campb303 requested changes Dec 15, 2020

View reviewed changes

wrigh393 added 2 commits December 16, 2020 14:38

Created ItemTableCell and LastUpdated cell components for refactoring…

4c57b61

… ItemTable

Fixed background color not displaying in LastUpdatedCell component

ae03e13

campb303 linked an issue Jan 4, 2021 that may be closed by this pull request

Add coloring to ItemTable dates to indicate age #90

Closed

campb303 mentioned this pull request Jan 4, 2021

Add coloring to ItemTable dates to indicate age #90

Closed

campb303 self-assigned this Jan 5, 2021

campb303 added this to the v1-readonly milestone Jan 5, 2021

campb303 added 13 commits January 19, 2021 21:35

Update Python requirements to fix Flask_JWT_Extended and PyJWT

8ac1d92

(cherry picked from commit 47e0715)

Update venv-manager to allow 60 second for requirement installation

7110cda

(cherry picked from commit 5dd1da9)

Moved ternary statements to swtich statement for readability

f36ac8e

Formatting

375b679

Rename reactTableProps to TableCellProps

55de973

Renamed folder and update exports

fc608a6

Formatting

0c3f1bb

Remove unused backgroundColor prop

813438a

Refactir timeToBackgroundColor and inject background color into props

672b0f9

Rename reactTableProps to ItemTableCellProps

86b75a1

Make time prop required

e3724de

Add docs

9df197e

Add docs

246e4d9

Update docs to show what values the time prop can be

1ed9daa

Merge branch 'staging' into enhancement-lastupdated-styling

1084d86

campb303 merged commit f3aa06f into staging Jan 22, 2021

Enhancement lastUpdated cell styling #135

Enhancement lastUpdated cell styling #135

wrigh393 commented Dec 3, 2020

campb303 commented Dec 3, 2020

campb303 left a comment

campb303 Dec 3, 2020

campb303 Dec 3, 2020

campb303 Dec 3, 2020

campb303 Dec 3, 2020

campb303 Dec 3, 2020

campb303 Dec 3, 2020

campb303 Dec 3, 2020

campb303 Dec 3, 2020

campb303 Dec 3, 2020

campb303 left a comment

campb303 Dec 15, 2020

campb303 Dec 15, 2020

campb303 commented Jan 21, 2021

	{...reactTableCellProps} //expands table cell props to allow access to style props
	// Expands table cell props to allow access to style props.
	{...reactTableCellProps}

	style={{ ...reactTableCellProps.style, backgroundColor: backgroundColor }} //expands style prop of table cell to allow addtion of custom styling without losing default styling
	// Expands style prop of table cell to allow addition of custom styling without losing default styling.
	style={{ ...reactTableCellProps.style, backgroundColor: backgroundColor }}

		@@ -135,9 +170,24 @@ export default function ItemTable({ data }) {
		selected={isSelected}
		{...row.getRowProps()} >

	//conditonally renders custom cell component based on cell.column.id prop value
	// Conditonally renders custom cell component based on cell.column.id prop value.

Enhancement lastUpdated cell styling #135

Enhancement lastUpdated cell styling #135

Conversation

wrigh393 commented Dec 3, 2020

campb303 commented Dec 3, 2020

campb303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

campb303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

campb303 commented Jan 21, 2021

Logical

Visual

Organizational